Conversation
🦋 Changeset detectedLatest commit: 798851b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a TreeView single-select edge case where multiple nodes can share the same data-path, causing click/keyboard selection to incorrectly target the first matching node in the DOM. The approach updates the TreeView behavior to act on the actual interacted element rather than re-resolving the node by path, and adds coverage + snapshots for the duplicate-path scenario.
Changes:
- Update single-select interactions to check the clicked/focused node element directly (avoiding path-based lookup collisions).
- Add a dedicated system test case and preview setup for duplicate-path nodes.
- Update Playwright ARIA snapshots and add a changeset documenting the patch.
Reviewed changes
Copilot reviewed 6 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
app/components/primer/alpha/tree_view/tree_view.ts |
Changes single-select logic to check the interacted node directly (but needs scoping fix for unchecking). |
previews/primer/alpha/tree_view_preview/single_select.html.erb |
Adds a second leaf with the same label/path to reproduce the issue in the preview. |
test/system/alpha/tree_view_test.rb |
Adds system tests asserting correct behavior when two nodes share the same path (mouse + keyboard). |
.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/tree_view/single_select/aria-snapshot.yml |
Updates ARIA snapshot for the extra duplicate node. |
.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/tree_view/single_select/aria-snapshot--after-interaction.yml |
Updates ARIA snapshot after interaction to reflect the extra node. |
.changeset/all-paths-dance.md |
Records the fix as a patch release note. |
myabc
left a comment
myabc
left a comment
There was a problem hiding this comment.
Two further findings on lines outside this diff (posted here since they can't be anchored inline):
Claude finding 🤖 — form submission is still ambiguous for duplicate paths (tree_view.ts#updateHiddenFormInputs, ~line 488)
Hidden form inputs serialize {path, value?}. With two leaves sharing a path and no data-value, selecting the second duplicate submits a payload identical to selecting the first — the server still cannot tell which node was selected. The ticket's wiki use case (root wiki nodes and sibling pages may share titles) is therefore fixed visually and in event details (which include node), but not end-to-end through form submission. A unique identifier (the data-node-id pattern already used by FilterableTreeView) or requiring data-value for duplicate-capable trees would close the gap.
Claude finding 🤖 — path-based APIs still resolve the first match (tree_view.ts#nodeAtPath, ~line 395)
markCurrentAtPath, toggleCheckedAtPath, checkedValueAtPath, disabledValueAtPath, expandAtPath/collapseAtPath/toggleAtPath, leafAtPath and the public checkOnlyAtPath all querySelector by data-path, so any consumer calling these with a duplicate path still targets the first match. Likewise TreeViewNodeInfo.path in event details remains ambiguous for consumers not using .node. Probably out of scope for this PR, but worth a follow-up ticket for id-based node identity.
18cb924 to
6a9395c
Compare
- Fix treeViewNodeChecked/Before events reporting wrong checkedValue on toggle-off (was hardcoded 'true', now reflects actual new state) - Align keyboard Space/Enter with click: pressing Space on an already-selected single-select node now deselects it - Include data-node-id as nodeId in hidden form input payload so duplicate-path nodes are distinguishable on the server side
c31e6b0 to
773e344
Compare
5d79f96 to
1f5d29e
Compare
59a4505 to
1f5d29e
Compare
There was a problem hiding this comment.
⚠️ Not ready to approve
The changeset version bump doesn’t align with the repo’s versioning guide for behavioral changes, and nodeId is not yet propagated to retained hidden-input payloads in async/filterable trees.
Copilot's findings
- Files reviewed: 6/7 changed files
- Comments generated: 2
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
b939d69 to
e00ca96
Compare
| @@ -271,7 +271,11 @@ export class TreeViewElement extends HTMLElement { | |||
| } else if (this.selectVariant(node) === 'single') { | |||
| event.preventDefault() | |||
There was a problem hiding this comment.
(pre-existing) it looks like single-select sets aria-checked without dispatching treeViewBeforeNodeChecked/treeViewNodeChecked.. so updateCheckedNodeIds won't end up being called.
myabc
left a comment
myabc
left a comment
There was a problem hiding this comment.
Looks good.
Worth fixing the keyboard selection issue - but this could happen in an separate PR.
What are you trying to accomplish?
Handle the case that two nodes have the exact same path
List the issues that this change affects.
https://community.openproject.org/wp/DREAM-704
Risk Assessment
What approach did you choose and why?
Keep the node object after selection instead of fetching it by the path
Root cause
In single-select mode, clicking a node triggered
checkOnlyAtPath(path), which re-looked up the target node. BecausequerySelectoralways returns the first matching element, clicking any node whose path was shared by an earlier node in the DOM would silently select that first node instead.Fix
Added a private
#checkNodeOnly(node: Element)method that unchecks all currently active nodes and checks the given element directly, bypassing the path-based lookup entirely. The publiccheckOnlyAtPathAPI is preserved and delegates to the same method.